Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(utils): Remove global buffer used when writing files to disk #442

Merged
merged 1 commit into from
Jan 16, 2020
Merged

fix(utils): Remove global buffer used when writing files to disk #442

merged 1 commit into from
Jan 16, 2020

Conversation

kattrali
Copy link
Contributor

@kattrali kattrali commented Jan 2, 2020

Fixes a case where shared memory buffer is used for writing to disk in a code path which can be accessed concurrently from multiple threads. Triggering the error on write requires calling notify from multiple threads at once while suspendThreadsForUserReported is disabled.

This change removes the buffer altogether, opting for a simpler solution where the system determines how/when to buffer when writing to disk. The buffer was added in kstenerud/KSCrash@be9bedb so the original intent is somewhat lost to the mists.

Tests

Tests for this are a bit fiddly since the issue is timing-based, though its possible to see the effect when using the reproduction case in #401 while setting suspendThreadsForUserReported = NO. Added an automated test for this case which would previously fail intermittently, as well as testing manually on iOS 12 / 13.

robinmacharg
robinmacharg reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a good opportunity to slightly expand the relevant KSFileUtils.h function header documentation to highlight the potential failure mode? On a (minor) related note: bsg_ksfureadBytesFromFD has an erroneous log message: BSG_KSLOG_ERROR("Could not write to fd...

robinmacharg
robinmacharg reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KSFileUtils_Tests.m has a commented out test: testWriteBytesToFDBig. Should this be reinstated, or removed at the same time as this change?

robinmacharg
robinmacharg reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an implicit (via the now-removed buffer declaration) 64K limit on the size of the passed in data that's been removed with this change. Is it worth stating/checking the expected size limits of passed in data?

robinmacharg
robinmacharg reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What alternative approaches (to removing the buffer) have been considered?

robinmacharg
robinmacharg reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concurrency, but related to the previous point: is there any possibility that an error in one thread's write() halfway through another's could affect operation? Are we safe avoiding semaphore's etc. in this case?

@kattrali
Copy link
Contributor Author

Is this a good opportunity to slightly expand the relevant KSFileUtils.h function header documentation to highlight the potential failure mode? On a (minor) related note: bsg_ksfureadBytesFromFD has an erroneous log message: BSG_KSLOG_ERROR("Could not write to fd...

Rebasing to apply the changes from #441, removing the erroneously documented functions.

KSFileUtils_Tests.m has a commented out test: testWriteBytesToFDBig. Should this be reinstated, or removed at the same time as this change?

This actually tested one of the unused functions, so its going away too.

There's an implicit (via the now-removed buffer declaration) 64K limit on the size of the passed in data that's been removed with this change. Is it worth stating/checking the expected size limits of passed in data?

I'd say no, because write(2) is smart enough to handle this case, and if any bytes remain after calling write(), then the bsg_ksfuwriteBytesToFD() function loops until all of the remaining data is written.

What alternative approaches (to removing the buffer) have been considered?

  • Add the new writing function only for notify() reports so the codepath did not need to change for crash reports. It seemed more difficult or maybe not worth the effort since the use of bsg_ksfuwriteBytesToFD() is deep in shared functions.
  • Locks/semaphores weren't really an option since this codepath is hit at crash time and needs to be async signal-safe.

Concurrency, but related to the previous point: is there any possibility that an error in one thread's write() halfway through another's could affect operation? Are we safe avoiding semaphore's etc. in this case?

Hopefully no:

  • In the notify() case, a new file path is generated for every report.
  • In the crash report case, all other threads are stopped (using bsg_kscrashsentry_suspendThreads()) as soon as the crash is detected, which should negate simulateous reports to the same file path.
  • If write() fails, it sets errno rather than raising or otherwise terminating ungracefully. errno is thread-local (1, 2, 3).

@kattrali
fix(utils): Remove global buffer used when writing files to disk
e906449 
write(2) already intelligently handles buffering/writing, so this buffer
is unneeded in addition to being unsafe for the user-initiated reporting
case.

The buffer was added in kstenerud/KSCrash@be9bedb
and thus the original intent is lost to the mists.
robinmacharg
robinmacharg approved these changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments - LGTM.

@kattrali kattrali merged commit 2f15bfc into master Jan 16, 2020
@kattrali kattrali deleted the kattrali/remove-shared-mem branch January 16, 2020 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robinmacharg robinmacharg robinmacharg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kattrali @robinmacharg